Adding support for .gitignore file like ignorefiles (and fix Windows build)#302
Adding support for .gitignore file like ignorefiles (and fix Windows build)#302WolverinDEV wants to merge 7 commits into
Conversation
sourcefrog
left a comment
There was a problem hiding this comment.
That's a great feature idea, and thanks for fixing the Windows build. Microsoft don't seem to be making it easy for me to have a working dev vm, but I don't mind taking patches for it.
| } | ||
|
|
||
| impl PathFilterOptions { | ||
| pub fn create_path_filter(&self) -> Result<Exclude> { |
There was a problem hiding this comment.
should we call this to_exclude?
There was a problem hiding this comment.
Personally I would prefer to rename Exclude to PathFilter as that name seems more appropriate for what it actually does now. However, that is something I wanted to discuss before proposing the change, since keeping the naming consistent would require renaming quite a few places throughout the codebase.
There was a problem hiding this comment.
Yes that sounds like an improvement, but let's consider it separately
Yeah, I saw the README. That's understandable. Sadly, I can't really provide any resources here either, as I'm personally on the very limited end as well 😆 In regards to your two comments, there is a significant distinction between The motivation behind this is the following: I do not, however, want conserve as a backup tool to unintuitively respect in-tree ignore files. Personally, even skipping directories containing CACHEDIR.TAG by default is already somewhat at the edge. In my opinion, a backup tool should, by default, back up the actual filesystem as it exists unless explicitly configured otherwise. Example ignorefile config# This is an excerpt of the ignore file I use for my server.
# Due to limited backup storage, I want fine-grained control over which data is actually backed up
# (e.g. databases and configuration files, but not cache or log files).
# Ignore everything but make the /home rules overwriteable
/*
!/home
/home/*
# Vaultwarden data
!/home/vaultwarden
/home/vaultwarden/vw-data/icon_cache
/home/vaultwarden/vw-data/tmp
# Traefik
!/home/traefik
/home/traefik/plugins-local
# Vikunja
!/home/vikunja
</details> |
Oh, OK, I see. Let's clarify that in the help text.
That makes sense.
I agree, even though build products can be large, there's a real risk of accidentally excluding something that you wish you'd kept. Possibly even the cachedir handling should be off by default. |
| exclude: Vec<String>, | ||
| #[arg(long, short = 'E')] | ||
| exclude_from: Vec<String>, | ||
| #[arg(long = "only", short = 'i')] |
There was a problem hiding this comment.
I think you erroneously deleted this line which is causing the test failures.
There was a problem hiding this comment.
🤦 how the hell did I miss that line in my own review of the changes....
Good catch!
There was a problem hiding this comment.
Short option names must be unique for each argument, but '-i' is in use by both 'ignore_file' and 'only_subtree'
Well that had be a very funny but yet unlucky circumstance.
Not sure how we should handle this conflict in this case.
| exclude_from: Vec<String>, | ||
|
|
||
| /// Specify a gitignore like ignorefile | ||
| #[arg(long, short)] |
There was a problem hiding this comment.
Let's just specify short = 'G' or something here, so that it doesn't default to i? Or even just leave off short for now.
|
|
||
| impl PathFilterOptions { | ||
| pub fn to_exclude(&self) -> Result<Exclude> { | ||
| if let Some(file) = &self.ignore_file { |
There was a problem hiding this comment.
I think we could potentially allow both and have them stack, but it doesn't have to be in this PR.
| } | ||
|
|
||
| pub fn from_ignorefile(file: impl AsRef<Path>) -> Result<Exclude> { | ||
| let mut builder = GitignoreBuilder::new("/"); |
There was a problem hiding this comment.
I think the effect of this is that the root of the backup is matched by the root of gitignore patterns: in the same place that the git tree root would normally match? That makes sense.
| #[arg(long, short = 'E', conflicts_with = "ignore_file")] | ||
| exclude_from: Vec<String>, | ||
|
|
||
| /// Specify a gitignore like ignorefile |
There was a problem hiding this comment.
| /// Specify a gitignore like ignorefile | |
| /// Exclude paths that match rules from a file with gitignore syntax. |
Hey ho,
it’s me again.
As the title suggests, this PR adds support for
.gitignore-style ignore files.Additionally combines two minor fixes / features: